Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

recent connect, updated config, passes tests #30

Merged
merged 5 commits into from
Jun 21, 2024
Merged

recent connect, updated config, passes tests #30

merged 5 commits into from
Jun 21, 2024

Conversation

aronatkins
Copy link
Contributor

@aronatkins aronatkins commented Jun 13, 2024 •
edited
Loading

  • Updates outdated actions.
  • Upgrades Connect to the most recent release.
  • Uses docker compose not docker-compose.
  • Wait for Connect to be healthy before up completes, which avoids a test-run race condition.
  • Remove PostgreSQL from the compose configuration in favor of SQLite, which made the test requirements lighter. Also avoids a Connect-DB race during startup.
  • Updates the automatic test API key.
  • Updates configuration to be more similar with the default included in the Docker image, but with "easy peasy testing".

Needed ahead of #29

@aronatkins
Copy link
Contributor Author

I updated the RSC_LICENSE (which expires in a year) hoping that was part of the CI problem, but no luck.

@aronatkins
Copy link
Contributor Author

May need to pull in changes from #26; not sure.

@aronatkins aronatkins requested a review from statik June 14, 2024 19:39
@aronatkins
Copy link
Contributor Author

Current state: I can run tests locally, but CI fails. Maybe Monday will offer inspiration.

@aronatkins
Copy link
Contributor Author

Resolved test startup races:

  1. Connect started before PostgreSQL. Removed PostgreSQL.
  2. Tests started before Connect. Added health-check and ask compose to wait.

@aronatkins aronatkins mentioned this pull request Jun 21, 2024
Merged
tdstein
tdstein approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@tdstein tdstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NodeJS 16.x is no longer under LTS. Should we upgrade the CI matrix to 18.x and 20.x?

@aronatkins
Copy link
Contributor Author

NodeJS 16.x is no longer under LTS. Should we upgrade the CI matrix to 18.x and 20.x?

Yes. That update is going to happen separately and is already started in #26. This PR is focused on getting CI passing, which other changes can build upon.

@tdstein
Copy link
Contributor

tdstein commented Jun 21, 2024

NodeJS 16.x is no longer under LTS. Should we upgrade the CI matrix to 18.x and 20.x?

Yes. That update is going to happen separately and is already started in #26. This PR is focused on getting CI passing, which other changes can build upon.

Perfect. Thanks!

meatballhat
meatballhat approved these changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🆙

@aronatkins aronatkins merged commit 7151f20 into main Jun 21, 2024
2 checks passed
@aronatkins aronatkins deleted the aron-passes-for-me branch June 21, 2024 15:31
@aronatkins aronatkins mentioned this pull request Jun 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meatballhat meatballhat meatballhat approved these changes

@tdstein tdstein tdstein approved these changes

@statik statik Awaiting requested review from statik

@toph-allen toph-allen Awaiting requested review from toph-allen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aronatkins @tdstein @meatballhat